Update fontsource integration according with the documentation#482
Closed
Update fontsource integration according with the documentation#482
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates the project’s fontsource integration to follow the new recommended SCSS utility-based API, wiring Poppins font usage through the shared fontsource mixins and adding the corresponding dependency along with lockfile updates. Class diagram for SCSS modules in updated fontsource integrationclassDiagram
class FontsourceUtilsScss {
<<module>>
+faces(metadata, weights, styles, axes)
}
class FontsourcePoppinsScss {
<<module>>
+$metadata
+faces(weights, styles)
}
class FontsScss {
<<scss_base>>
+use_fontsource_utils()
+use_poppins_mixins()
+include_poppins_faces_normal()
+include_poppins_faces_italic()
}
FontsScss ..> FontsourceUtilsScss : uses
FontsScss ..> FontsourcePoppinsScss : uses
FontsourceUtilsScss ..> FontsourcePoppinsScss : reads metadata
Flow diagram for updated fontsource SCSS integrationflowchart LR
A[Install dependencies via package.json] --> B[at fontsource-utils/scss installed]
A --> C[at fontsource/poppins installed]
B --> D[SCSS file _fonts.scss]
C --> D
subgraph NodeModules
B
C
end
D --> E[at use fontsource mixins]
D --> F[at use Poppins mixins]
E --> G[at include fontsource.faces with Poppins.$metadata normal]
E --> H[at include fontsource.faces with Poppins.$metadata italic]
G --> I[Generated at font-face rules for Poppins normal]
H --> J[Generated at font-face rules for Poppins italic]
I --> K[Compiled CSS output]
J --> K
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Using hardcoded
../../../node_modules/...paths in the SCSS@usestatements is brittle; consider relying on package resolution (e.g.@use '@fontsource-utils/scss/src/mixins') or your bundler’s include paths instead. - The addition of
addandyarntodependencieslooks unrelated to the fontsource change and may be unnecessary runtime dependencies; double-check whether they can be omitted or moved to devDependencies.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using hardcoded `../../../node_modules/...` paths in the SCSS `@use` statements is brittle; consider relying on package resolution (e.g. `@use '@fontsource-utils/scss/src/mixins'`) or your bundler’s include paths instead.
- The addition of `add` and `yarn` to `dependencies` looks unrelated to the fontsource change and may be unnecessary runtime dependencies; double-check whether they can be omitted or moved to devDependencies.
## Individual Comments
### Comment 1
<location path="package.json" line_range="21" />
<code_context>
"dependencies": {
+ "@fontsource-utils/scss": "^0.2.2",
"@fontsource/poppins": "^5.0.5",
+ "add": "^2.0.6",
"lazysizes": "^5.3.2",
- "oneloop.js": "^5.2.1"
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Reconsider introducing the generic `add` package as a dependency unless it’s clearly needed.
This package is very small and commonly used only in examples. Adding it as a runtime dependency increases your dependency surface (and potential vulnerability exposure) for limited gain. If it’s only needed for build tooling, consider moving it to `devDependencies`, or omitting it entirely if it’s not essential.
Suggested implementation:
```
"@fontsource-utils/scss": "^0.2.2",
"@fontsource/poppins": "^5.0.5",
"lazysizes": "^5.3.2",
```
1. If the `add` package is actually required for build tooling (e.g., used in scripts or config), reintroduce it under `"devDependencies"` instead of `"dependencies"`.
2. Run `yarn install` or `npm install` after updating `package.json` to sync the lockfile (`yarn.lock` or `package-lock.json`) and include that in the PR as well.
</issue_to_address>
### Comment 2
<location path="package.json" line_range="24" />
<code_context>
"lazysizes": "^5.3.2",
- "oneloop.js": "^5.2.1"
+ "oneloop.js": "^5.2.1",
+ "yarn": "^1.22.22"
},
"devDependencies": {
</code_context>
<issue_to_address>
**🚨 issue (security):** Avoid listing `yarn` as an application dependency.
Putting `yarn` in `dependencies` installs it in production even though it’s only a dev tool, increasing install time and attack surface. Please remove it from `dependencies`; only use `devDependencies` (or external documentation) if you truly need to call it explicitly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| "dependencies": { | ||
| "@fontsource-utils/scss": "^0.2.2", | ||
| "@fontsource/poppins": "^5.0.5", | ||
| "add": "^2.0.6", |
There was a problem hiding this comment.
🚨 suggestion (security): Reconsider introducing the generic add package as a dependency unless it’s clearly needed.
This package is very small and commonly used only in examples. Adding it as a runtime dependency increases your dependency surface (and potential vulnerability exposure) for limited gain. If it’s only needed for build tooling, consider moving it to devDependencies, or omitting it entirely if it’s not essential.
Suggested implementation:
"@fontsource-utils/scss": "^0.2.2",
"@fontsource/poppins": "^5.0.5",
"lazysizes": "^5.3.2",
- If the
addpackage is actually required for build tooling (e.g., used in scripts or config), reintroduce it under"devDependencies"instead of"dependencies". - Run
yarn installornpm installafter updatingpackage.jsonto sync the lockfile (yarn.lockorpackage-lock.json) and include that in the PR as well.
| "lazysizes": "^5.3.2", | ||
| "oneloop.js": "^5.2.1" | ||
| "oneloop.js": "^5.2.1", | ||
| "yarn": "^1.22.22" |
There was a problem hiding this comment.
🚨 issue (security): Avoid listing yarn as an application dependency.
Putting yarn in dependencies installs it in production even though it’s only a dev tool, increasing install time and attack surface. Please remove it from dependencies; only use devDependencies (or external documentation) if you truly need to call it explicitly.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Accidental
addandyarnpackages committed as dependencies- Removed accidental
addandyarnentries fromdependenciesand regeneratedyarn.lockto fully drop both packages from the dependency graph.
- Removed accidental
Or push these changes by commenting:
@cursor push a4a7ac7a89
This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f399a33. Configure here.
| "lazysizes": "^5.3.2", | ||
| "oneloop.js": "^5.2.1" | ||
| "oneloop.js": "^5.2.1", | ||
| "yarn": "^1.22.22" |
There was a problem hiding this comment.
Accidental add and yarn packages committed as dependencies
Medium Severity
The "add": "^2.0.6" and "yarn": "^1.22.22" entries in dependencies are spurious packages accidentally introduced when installing @fontsource-utils/scss. The add package is an unrelated floating-point math library, and yarn is the package manager itself (already managed via packageManager and volta fields). Neither is imported or used anywhere in the project source code. These bloat the production dependency tree unnecessarily and could cause confusion or conflicts.
Reviewed by Cursor Bugbot for commit f399a33. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Mise à jour de l'intégration de font source via l'utilitaire recommandé par la documentation. Font source a évolué entre temps.
Summary by Sourcery
Update font loading to use the recommended @fontsource-utils SCSS mixins for defining font faces.
New Features:
Enhancements:
Build:
Note
Medium Risk
Medium risk: updates font-face generation and adds new runtime dependencies, which can subtly change compiled CSS/font loading and bundling output.
Overview
Switches Fontsource SCSS integration to the recommended
@fontsource-utils/scssfacesmixin, passing font$metadatainstead of calling the font package’sfacesdirectly.Updates dependencies/lockfile to include
@fontsource-utils/scss(and addsaddandyarntodependencies), which may affect install/bundle behavior.Reviewed by Cursor Bugbot for commit f399a33. Bugbot is set up for automated code reviews on this repo. Configure here.